refactor(TileBuilder): migrate to TypeScript #2440
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
3 times, most recently
from
c131f60 to
9d9b3c8
Compare
HoloTheDrunk
changed the title
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
4 times, most recently
from
164fabe to
d8855a9
Compare
|
It'd be great if I could please get a go-ahead on the general structure of this refactor so I can start working on the next one—which will inevitably be based off of this one, while the specific details of the PR get reviewed :) |
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
from
d8855a9 to
5dad0cb
Compare
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
from
5dad0cb to
55a01e7
Compare
jailln
reviewed
Nov 26, 2024
There was a problem hiding this comment.
Thanks for all these changes, it's already better and easier to understand with typings. I think we should take this opportunity to improve the architecture and the doc of this part of the code a little bit more, that's what stands out from my comments. Let me know what you think :)
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
from
7e84c08 to
865316d
Compare
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
2 times, most recently
from
c1c0dce to
c4f597b
Compare
jailln
approved these changes
Dec 3, 2024
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
3 times, most recently
from
ca84bba to
f5cafb7
Compare
Improve performance by using statically-sized ArrayBuffers. Reorganize code to get rid of some of the params/builder mess. Cleanup computeBuffers function. Squashed commit history (oldest to youngest): - fix: UV_1 generation - refacto(wip): cleanup and optimize computeBuffers - refacto(wip): improve index generation - wip: correct offset, off-by-one still at large - fix: change calls to allow camera debug - fix: found the error, off by a power of 2 actually - fix(uv): correct indices passed to UV buffering - fix(index): only generate buffer when needed - wip: enable cache - fix(computeBuffers): group tile and skirt together - fix(wip): squash rogue private field access - refacto: convert TileGeometry to TypeScript - style(builders): make method visibility explicit - refactor(exports): remove default exports - feat(TileGeometry): add OBB type - refactor: rename BuilderEllipsoidTile, fix imports - fix(tileGeometry): update tests to use public api - fix(TileBuilder): remove dead code comments
feat(tile): add doc comments, rm unused tileCenter
HoloTheDrunk
force-pushed
the
tile_builder_typescript
branch
from
f5cafb7 to
4fc027e
Compare
HoloTheDrunk
changed the title
HoloTheDrunk
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Create types for the tile builder section of iTowns.
Improve performance of tile creation.
Improve code readability of tile creation.
Depends on #2447.Merged.Summary of changes
BuilderEllipsoidTilebeing changed toGlobalTileBuilderto match the naming of thePlanarTileBuilder(especially considering its position inside aGlobefolder). Hesitated between "Globe" and "Global" so feel free to criticize the choice, it's already a breaking change so we might as well pick the best one.computeBuffers.jsintocomputeBuffers.ts, changing algorithm to take advantage of simple math to avoid using slow JS constructs in favor ofArrayBuffers and moving some checks out of loops.Motivation and Context
This fits into the larger motion towards converting iTowns to TypeScript.
Code had to be changed and moved around to allow static type checking.
Some of the functions around buffer generation were cryptic and had low readability, which required additional changes.
Tested on Ubuntu 22.04.5 LTS x86_64 using Firefox 126.0.1 (64-bit)